The field DecodedInstruction::rs2 is specifically the bits 20:25 of the instruction, interpreted differently depending on the instruction type. In fact this PR still uses it as part of imm_i.

If you want to change something, I suggest just renaming the fields (e.g. rs2 -> bits_20_25).

I suppose the naming scheme was this. First the instruction is decoded into all fields besides immediate pieces. Most visible in the R-type. Then these fields are either interpreted as what they’re named after, or they are combined in one of the immediate format.

The subset of RiscV instructions we picked happen to all have either no rs2 specified or have their rs2 in the same location.

Other RiscV instructions have their rs2 in different locations.

If we want to have some abstraction and meaning, then the field rs2 in the DecodedInstruction should stand for the index of the second input register of that instruction, no matter where that information is located in the bits. If the instruction in question doesn't use a second input register, rs2 should be None or 0 or so, not some random bits.

If we want the field rs2 to mean 'give me some arbitrary bits without regard to their meaning', then I agree with your suggestion for a rename.

But why stop there? If we just want to use the bits as bits, what's the point of having DecodedInstruction at all? Just use a naked u32 and be done with it.

Keep in mind you are looking at private details of a decoder here. It handles bit fields of some data format; it knows nothing about the VM. Variables have names as suggested by the diagram (I presume).